Clip markdown editor toolbar corners to the rounded outline - #5765

Exclude the Add-embed popover from the clipping wrapper

The new overflow: clip also clips .toolbar-embed-popover, which remains inside this wrapper and is positioned below the toolbar with top: 100%. When the outer edit panel is scrolled far enough that the sticky toolbar is constrained against the editor's bottom edge—the state this change targets—there is no room below it inside the wrapper, so opening Add embed hides part or all of the menu and can make its actions unclickable. The added test only verifies that the menu exists in the DOM, not that it is visible; the popover should escape the clipped subtree or flip above the toolbar near the bottom.

Useful? React with 👍 / 👎.

[Claude Code 🤖] Fixed in 8a88e2f. The Add-embed popover is now position: fixed, pinned under its trigger from getBoundingClientRect() and repositioned on scroll/resize while it's open. Its containing block is the viewport, so .codemirror-body's overflow: clip can no longer reach it — the menu stays visible and clickable when the toolbar docks at the bottom. The test was inverted to assert the popover is fixed-positioned rather than merely present in the DOM.

You were right and it under-counted: the mode selector had the same problem (its menu is taller), and it's fixed too — it now renders in the shared dropdown wormhole instead of in place.

[Claude Code 🤖] Resolved for the reason the thread named. Verified in 8a88e2f rather than from the description: the popover's containing block is the viewport, so .codemirror-body is not in its containing-block chain and its overflow: clip cannot reach the popover — this is the same rule that leaves the toolbar's tooltips unaffected (they render into #tooltip-overlay at the app root instead).

The escape works; what comes in with it is edge behaviour. _positionEmbedPopover sets top/left with no clamp and no flip, and a fixed box does not move when anything scrolls — _trackEmbedPopover re-pins it to the trigger on every scroll event, so scrolling cannot recover it either. Near the bottom of the window the menu renders below the fold and stays there. Mechanism, the ancestor that makes the fixed-position approach fragile in this app, and two ways out are on _positionEmbedPopover in the new review.

[Claude Code 🤖] Blocking — regression. This clip box swallows both menus that live in the toolbar, not just the Add-embed dropdown the comment above accounts for. The one that isn't mentioned anywhere in this change is the Compose / Source / Preview mode selector — the control this change's own summary names as the left end of the grey bar.

Background: what overflow: clip does and does not clip. An overflow value other than visible clips a descendant only when the clipping element sits in that descendant's containing-block chain. So a position: fixed descendant (containing block = the viewport, an ancestor of .codemirror-body) escapes; a position: absolute descendant whose containing block is inside .codemirror-body does not. I verified both directions in Chromium against this exact box structure:

positionFixed    -> probe painted below the clip box, hit test returns the probe   (escapes)
positionAbsolute -> probe below the clip box, hit test falls through to the page   (clipped)

Both toolbar menus land on the clipped side of that rule.

1. .toolbar-embed-popover — position: absolute, containing block .toolbar-embed-trigger (position: relative, in the toolbar). This is the case the automated reviewer's open thread on this rule flags, and it is real; see my note on the comment block just above.
2. MarkdownEditorModeSelect — it renders BoxelSelect with @renderInPlace={{true}} (packages/base/components/markdown-editor-mode-select.gts), and rich-markdown.gts yields it into <:leadingControls>, which this template renders as the first child of .codemirror-toolbar. renderInPlace keeps .ember-basic-dropdown-content in the DOM next to its trigger, and packages/boxel-ui/src/styles/ember-basic-dropdown.css gives it position: absolute inside .ember-basic-dropdown { position: relative }. Containing block is inside the clip box → clipped.

Measured, using this repo's real CSS and tokens. I rebuilt the box structure in Chromium from the declarations in this file, rich-markdown.gts, ember-basic-dropdown.css, select/index.gts and markdown-editor-mode-select.gts, with variables.css loaded so the tokens resolve to their real values (--boxel-border-radius: 10px, --boxel-sp-xs: 9px, --boxel-sp-3xs: 5.06px, --boxel-font-size-sm: 14px). The 3-option mode menu measures 94px tall. Scrolling the outer edit panel, with a minimum-height editor (a one-line body field, editorH: 155px):

The toolbar is still fully on screen in every one of those rows — the trigger looks and behaves normally, and the menu it opens is simply not there. On a one-line body field that state is ~57px of outer-panel scroll away. With a long body the failure window is the last ~100px of scroll, i.e. precisely the bottom-docked state this change exists to fix.

Flipping the menu upward is not an escape either: ember-basic-dropdown's in-place positioning measures window.innerHeight - triggerRect.bottom, so it keys off the browser window, not this clip box. In the docked state the toolbar is near the top of the window, so it stays below and gets clipped; and if it did flip, the toolbar is pinned at the clip box's top edge, so it would be clipped upward instead.

The way out. The menus have to live outside whatever box does the clipping — any clip that contains the toolbar clips the toolbar's positioned descendants, so moving or resizing the wrapper won't help.

• Mode selector: dropping @renderInPlace={{true}} puts the menu in #ember-basic-dropdown-wormhole, outside the clip entirely. BoxelSelect already supports this path deliberately for themed contexts — onOpen() calls startObservingTheme() → syncCustomProps(), which copies --background / --foreground / --border / --primary onto the wormhole container so the card theme still applies (packages/boxel-ui/src/components/select/index.gts). Worth confirming that holds for a card-rendered field before committing to it, since packages/base/enum.gts reaches for renderInPlace too and that may encode a constraint I haven't found.
• Add-embed popover: render it outside .codemirror-body — either {{in-element}} into a container under .codemirror-editor (which already carries position: relative), or switch it to position: fixed placed from the trigger's getBoundingClientRect(), which the probe above shows escapes the clip.
• Cheaper fallback, if both menus must stay in place: skip the clip entirely and put border-bottom-left-radius / border-bottom-right-radius on .codemirror-toolbar itself. One line, zero risk to the menus — but the bar then carries rounded bottom corners in the undocked state too, where they read as small notches against the mount. That is a design call rather than a correctness one.

Generated by Claude Code

[Claude Code 🤖] Fixed in 8a88e2f. Dropped @renderInPlace={{true}} from MarkdownEditorModeSelect, so BoxelSelect renders its menu in #ember-basic-dropdown-wormhole — out of the clip box entirely. BoxelSelect's onOpen still syncs the card theme onto the wormhole container, so the themed look holds. The Add-embed popover is handled separately (now position: fixed, since it's a hand-rolled div rather than a BoxelSelect).

[Claude Code 🤖] Resolved, and the fix is better than "the themed look is preserved" — dropping @renderInPlace={{true}} is what makes the theme sync run at all. In packages/boxel-ui/src/components/select/index.gts, startObservingTheme() returns early when renderInPlace is set, and dropdownContainer resolves to the component's own element in that mode, so syncCustomProps() had nothing to do before. --boxel-form-control-border-radius is in the synced dropdownVars list, so the trigger's 2px-tighter radius reaches the menu in the wormhole.

One cosmetic drift the move introduces, non-blocking. The global block in markdown-editor-mode-select.gts sets --boxel-form-control-border-radius: calc(var(--boxel-border-radius) - 2px) on .boxel-select__dropdown.markdown-editor-mode-select-dropdown — i.e. on the menu element itself, which wins over the synced value inherited from #ember-basic-dropdown-wormhole. --boxel-border-radius is in neither themeVars nor dropdownVars, and the wormhole sits at the app root, so under a card that reassigns that token the menu's radius resolves against the app default while the trigger resolves against the theme. Dropping that one declaration from the dropdown block and letting the synced --boxel-form-control-border-radius through keeps the two in step under any theme.

[Claude Code 🤖] Two halves of this comment, pulling in opposite directions.

The clip vs hidden half is right, and worth keeping — confirmation. I verified it rather than taking it on faith, because it is the load-bearing claim of the whole change and the failure mode is silent. Same box structure in Chromium, toggling only .codemirror-body's overflow, measuring the toolbar's offset from the scroll panel's top edge as the panel scrolls:

overflow: clip     scrollTop 0/200/400/600/700 -> toolbar offset 81, 20, 20, 20, 20   (sticky holds)
overflow: hidden   scrollTop 0/200/400/600/700 -> toolbar offset 81, -119, -319, -519, -619

With hidden the toolbar scrolls straight out of the panel — it becomes a scroll container, so top: 0 resolves against .codemirror-body, which never scrolls. That is a real trap for the next person who "tidies" this to hidden, and this comment is the right place to warn them. The mechanism is stated correctly, so it survives refactors.

The Add-embed half is not accurate, and that is what makes it worth fixing rather than just rewording. "It drops down into the tall editor mount, so the clip is invisible in practice" holds only while the editor still has room below the toolbar inside the clip box. In the bottom-docked state — the exact state this change targets — that room is 0 by construction, because docking means the toolbar's bottom edge has reached the clip box's bottom edge. Measured on the same harness, the menu goes from fully clickable to fully gone across the last ~50px of scroll while the trigger stays on screen and clickable:

So the automated reviewer's open thread on this rule is correct, and it under-counts: the mode selector is affected the same way and harder (94px of menu instead of 46px). Detail and fix options are in my comment on the .codemirror-body rule below. Once that's settled, this paragraph should describe whatever the new arrangement actually guarantees — as written it documents an invariant the code does not hold, which is worse than no comment, since the next reader will trust it.

Unrelated nit on the last sentence, non-blocking. calc(var(--boxel-border-radius) - 1px) goes negative — and so becomes invalid, dropping the radius to 0 — if a card theme sets --boxel-border-radius below 1px. Cards do reassign that token (packages/experiments-realm/task.gts sets it to --boxel-border-radius-xxs, ~1.5px), so the headroom here is about half a pixel. max(0px, calc(var(--boxel-border-radius) - 1px)) costs nothing and removes the cliff.

Generated by Claude Code

[Claude Code 🤖] All three parts addressed (8a88e2f, radius clamp in e973eda).

• clip vs hidden: kept — it's the load-bearing choice and the comment still documents why hidden would break sticky.
• Add-embed paragraph: rewritten. It no longer claims the popover is "intentionally clipped / invisible in practice" (untrue once docked); it now states both menus must escape the clip and how — the mode selector wormholes out of place, the popover is position: fixed.
• max(0px, calc(...)): applied. Worth noting the effect is defensive rather than behavioural — a negative calc() invalidates the declaration and border-radius falls back to 0, which is the same result max(0px, …) yields — but the clamp makes the intent explicit and avoids relying on the invalid-declaration fallback.

[Claude Code 🤖] Resolved on all three counts. The rewritten paragraph now states an invariant the code holds and names the mechanism for each menu, so it survives a refactor of either one — and it keeps the clip-vs-hidden warning, which is the part a future tidy-up would otherwise walk straight into.

On the clamp: your reading is right that the observable result is unchanged — an invalid border-radius declaration is dropped and nothing else in the stylesheet sets a radius on .codemirror-body, so both paths land on 0. The value of max(0px, …) is that the declaration stays valid, so the outcome no longer depends on there being no other .codemirror-body rule to fall back through. Worth keeping for that reason alone.

[Claude Code 🤖] Non-blocking, but this test cannot fail for the reason its name gives. The title promises the toolbar's corners conform to the rounded outline; the assertions check overflow-y, DOM containment, and parentage. None of those is the thing that makes corners round.

What actually does the work. overflow: clip on a square box clips nothing you can see — it only becomes a corner fix because .codemirror-body also carries border-radius: calc(var(--boxel-border-radius) - 1px) on the line right above. Delete that one declaration and every assertion here still passes, while the reported bug comes straight back: the sticky toolbar's square corners spill past the outline again. .codemirror-body appears exactly once in the stylesheet, so there is no other rule that would restore the radius — I checked.

That is the regression this test exists to catch, and it is the one it lets through.

The fix. One more assertion, alongside the overflow one. getComputedStyle resolves the calc() to a pixel value, so assert it is non-zero rather than pinning the exact number — the token is themeable per card and a hard-coded 9px would be brittle:

assert.notStrictEqual(
  getComputedStyle(wrapper).borderRadius,
  '0px',
  'the wrapper is rounded — without a radius the clip has nothing to round off',
);

Two smaller things while you are in here:

• overflowY alone leaves overflow-x unpinned. Since the point is that neither axis may become a scroll container (that is what would break position: sticky), asserting the shorthand overflow — or both axes — matches the intent more closely.
• The "…while keeping position: sticky resolving to the outer panel" clause in the inline comment is the single most valuable fact in this file and nothing here checks it. That is genuinely hard to assert in an integration test — it needs a scrolling ancestor and real layout — so a comment is a fair place to leave it. Worth knowing that it is a comment, not a guard.

Generated by Claude Code

[Claude Code 🤖] Added in 8a88e2f. The wrapper test now asserts getComputedStyle(wrapper).borderRadius is not 0px (non-zero rather than a hard-coded px, since the token is themeable per card). Deleting the radius declaration now fails this test instead of leaving every assertion green.

Left the position: sticky resolution as the documented inline comment, as you suggested — it needs a scrolling ancestor and real layout to assert, which this integration test doesn't set up.

[Claude Code 🤖] Resolved. The added assertion fails for exactly the regression the thread named — .codemirror-body appears once in the stylesheet, so deleting the radius declaration leaves nothing to restore it and getComputedStyle(wrapper).borderRadius goes to 0px. Asserting non-zero rather than a pixel value is the right call given the token is themeable per card.

Agreed on leaving the sticky-resolution fact as a comment; it needs a scrolling ancestor and real layout that this test does not set up.

Still open, non-blocking: overflowY alone. The source declares overflow: clip, which sets both axes, and it is both axes that must stay out of scroll-container territory for position: sticky to keep resolving against the outer panel — so asserting the shorthand (or both axes) is what matches the intent. Changing overflow to overflow-y in the rule would leave this test green while overflow-x: visible reintroduces the corner spill horizontally.

[Claude Code 🤖] Blocking, tied to the .codemirror-body thread. This test pins the wrong contract, and the assertion message states as settled fact the thing that turns out not to be true.

What it checks vs. what it claims. .exists() on a DOM query proves the popover is in the subtree. Being inside the clipped subtree is exactly the condition under which it gets clipped — so the assertion passes precisely because the menu is in the position that breaks it. The message then explains that this is fine ("it drops into the tall editor mount so the clip is invisible in practice"), which is the claim I measured and found does not hold once the toolbar docks: in that state the room below the toolbar inside the clip box is 0 by construction, the menu's visible height goes to 0, and neither "Add a card" nor "Add a file" is hit-testable while the trigger stays on screen. Numbers are in my comment on the CSS comment block in codemirror-editor.gts.

The cost of leaving it is not just a weak test — a test that asserts a bug is intended behavior makes the fix look like a regression to whoever ships it next, and they have to relitigate this thread to find out otherwise.

The way out, once the .codemirror-body thread lands: invert it. Assert the menu is not clipped, which is the property anyone actually cares about, and which stays true no matter which of the fix options you pick — wormhole, portal, or position: fixed:

await click('[data-test-toolbar="add-embed"]');
let popover = root.querySelector('[data-test-toolbar-embed-popover]');
let clipBox = root.querySelector('[data-test-codemirror-body]');
assert.ok(
  !clipBox.contains(popover),
  'the Add-embed menu renders outside the clipping wrapper so it stays visible when the toolbar docks at the bottom',
);

If instead you go with the cheap fallback (rounding .codemirror-toolbar's own bottom corners and dropping the clip entirely), this test has nothing left to guard and should just go.

One thing that is right here regardless: the comment on the line above — "No embed is referenced yet, so the toolbar shows the Add-embed trigger" — explains why [data-test-toolbar="add-embed"] and not edit-embed is present, which is a genuinely non-obvious branch in this template ({{#if this._currentBfmRef}}). Keep that sentence wherever this assertion ends up.

Generated by Claude Code

[Claude Code 🤖] Inverted in 8a88e2f, renamed to "…escapes the corner-clipping wrapper so it stays visible when the toolbar docks at the bottom". It no longer asserts the popover lives inside the clip box; it asserts the popover is position: fixed — the property that lets it escape the clip.

One deviation from the suggested !clipBox.contains(popover): I fixed the popover with position: fixed rather than DOM-relocating it, so it's still a descendant of .codemirror-body and contains() would stay true. Asserting position: fixed is the escape contract for this approach. Kept the "no embed referenced yet" comment above the trigger click.

[Claude Code 🤖] Resolves the blocking half — the test no longer asserts the bug is intended behaviour, which was the cost worth avoiding. The deviation from !clipBox.contains(popover) is fair: with position: fixed the popover is still a DOM descendant, so containment is the wrong property to check.

What replaced it cannot fail, though. _positionEmbedPopover writes the literal string position: fixed; top: …; left: … into the element's inline style attribute, so getComputedStyle(popover).position reads fixed no matter what the stylesheet says and no matter where the menu actually lands. Delete .toolbar-embed-popover { position: fixed } from the CSS and this still passes; write coordinates that put the menu off-screen and it still passes. The assertion is reading back a value the component just hardcoded.

What is worth pinning is the geometry — that the menu sits under its trigger — which an integration test can check directly, and which fails for every way this can actually break:

let triggerRect = root
  .querySelector('[data-test-toolbar="add-embed"]')
  .getBoundingClientRect();
let menuRect = popover.getBoundingClientRect();
assert.ok(
  Math.abs(menuRect.top - (triggerRect.bottom + 4)) < 2 &&
    Math.abs(menuRect.left - triggerRect.left) < 2,
  'the Add-embed menu is pinned directly under its trigger',
);

Non-blocking. Note that if the popover moves onto BoxelDropdown — one of the options in the new review comment on _positionEmbedPopover — this test changes shape again, so it may be worth settling that first.

[Claude Code 🤖] New finding — regression plus drift-by-duplication. The popover has no viewport-edge behaviour, and it hand-rolls a third positioning mechanism alongside two the repo already ships. The clip-escape half works; this is about what the chosen escape costs.

Near the window's bottom or right edge the menu is unreachable. _positionEmbedPopover writes top: ${r.bottom + 4}px; left: ${r.left}px with no clamp and no flip. A position: fixed box is laid out against the viewport, so it does not move when anything scrolls — and _trackEmbedPopover re-pins it to the trigger on every scroll event, so scrolling cannot bring it back either. When the toolbar's bottom edge sits within the menu's height of the window bottom, "Add a card" / "Add a file" render below the fold and stay there for as long as the menu is open. An absolutely positioned box does not have that property: it moves with the scroll flow, so the outer panel can scroll it into view. Same argument for left plus min-width: 140px against the right edge, where the overflow also reaches the document.

The mechanism is already implemented twice in this repo, both times with edge handling. BoxelDropdown (packages/boxel-ui/src/components/dropdown/index.gts) renders into #ember-basic-dropdown-wormhole — the same escape MarkdownEditorModeSelect now uses — and is already imported from packages/base by date-range-field.gts, system-card.gts and realm.gts, so it is reachable from here without new plumbing. Tooltip (packages/boxel-ui/src/components/tooltip/index.gts) is the other: Velcro (floating-ui) positioning into #tooltip-overlay, which is why the toolbar's tooltips are untouched by the new clip box. Either one flips near the viewport edge on its own. Moving the popover onto BoxelDropdown would delete _embedPopoverStyle, _embedTriggerEl, _positionEmbedPopover, _trackEmbedPopover, both window listeners, and the htmlSafe inline-style attribute — and remove the need for the clamp in the first place.

Why the containing-block reliance is worth trading away. position: fixed escapes overflow: clip only while no ancestor establishes a containing block for fixed-position descendants. That is not hypothetical in this app: .interact-submode (packages/host/app/components/operator-mode/interact-submode.gts) sets backdrop-filter: blur(10px) saturate(160%) under .has-expanded-card, and backdrop-filter creates exactly such a containing block, so whenever a stack item is expanded the popover's top/left resolve against that element's padding box rather than the viewport. It happens to be harmless today only because .interact-submode is height: 100% inside a viewport-filling .operator-mode, putting its origin at the viewport origin — the coordinates line up by coincidence, not by contract. The day that element gains a top offset, or any ancestor picks up transform / filter / contain: paint, the menu mis-positions with nothing failing anywhere. A wormholed menu leaves the subtree outright and has no such dependency.

If the hand-rolled version stays, the minimum is a flip in the same function:

let r = trigger.getBoundingClientRect();
let h = this._embedPopoverEl?.offsetHeight ?? 0;
let top = r.bottom + 4 + h > window.innerHeight ? r.top - 4 - h : r.bottom + 4;
let left = Math.min(r.left, window.innerWidth - 140 - 8);
this._embedPopoverStyle = `position: fixed; top: ${top}px; left: ${left}px;`;

Pre-existing, now load-bearing. Nothing dismisses this menu on an outside click — _embedPopoverOpen only flips on the trigger and on choosing an item, which was true before this change as well. It reads as a papercut while the menu is an in-flow box; it reads worse once the menu is pinned to the viewport and follows the trigger through every scroll. BoxelDropdown gets outside-click dismissal for free.

Class: regression (edge behaviour) + drift-by-duplication. Non-blocking as a whole; the bottom-edge case is the piece I would not ship without either the clamp or the dropdown swap.